-
Notifications
You must be signed in to change notification settings - Fork 0
[WIP] 『最新の情報を取得』 #14
base: master
Are you sure you want to change the base?
[WIP] 『最新の情報を取得』 #14
Conversation
厳しいことを言ってしまうと、大体間違ってるのでコメントどうしようか悩むレベル。 |
う、、、すいません。まずそもそもの方針をしっかりと理解したいです。 |
最も大きな問題はテーブルの作り方が間違ってる、つまり、データ構造の捉え方が間違っている。 # FeedはChannelに名前変更したほうが分かりやすそう
class Feed < ApplicationRecord
# title, descriptionを追加
# attribute :url, :title, :description
has_many :items
end
class Item
# attribute :title, :link, :description
belongs_to :feed
end |
app/controllers/feeds_controller.rb
Outdated
@@ -21,6 +21,7 @@ def create | |||
|
|||
private | |||
def feed_params | |||
params.require(:feed).permit(:url) | |||
params.require(:feed).permit(:url, channel_attributes: [:channel_title, :description, :item_title, :link, :pubdate, :feed_id, :id]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strong parameterがなんで生まれたのかの小話。
Railsだとフォームとかで送られてきたパラメータを例えば User.new(params[:user])
みたいに雑に受けるのが昔は一般的だったのですが、これの問題として、ユーザー操作によって更新されると困るものまで操作されえてしまう、という問題がありました。
フォームでしか情報更新したことないとよくわからないかもしれませんが、ブラウザ上でHTMLをいじればユーザーが好きなnameのフォームも作れますし、また、curlなどを使って直接URL叩いてフォーム送信することもできるので、例えばusersにadminというカラムがあってそれで管理者権限を管理している場合など、 admin: true
なデータを勝手に送られると、意図せずユーザーに管理者権限を取られてしまうことがありえました。
Rails3までは attr_accessible
という機能を使ってこれを防いでいましたがちょっと機能的によくなかった面もあり、今はStrong Parameterというのを使っています。(この、 params.permit.require
ってやつ )
で、このコードだと id
なんかがstrong parameterでpermitしていますけれども、これって要するに「ユーザーがidを勝手に変更していい」という意思表示になるわけで、そんなことを許すことは絶対にないので書いちゃダメです
データの捉え方が間違ってるの、回り道っぽいですけれども着実な手段として、まずは全てのデータを一つのテーブルで表現してみて、そこから第3正規化くらいまでしてみるのがいいと思います。 |
正規化をしていく過程は「頭で考えるだけ」ってのはやめたほうがいいです。紙でもスプレッドシートでもなんでもいいので、書き出していくのが大事 |
いつも丁寧にすいません:sweat_drops: 今、idを書く危険性を知りもしなかったことに危険性を感じています:sweat: |
フォームなどで入力して、paramsから取り出して、DBに入れる、という流れしか経験しておらず、そこからの応用ができるほどにはまだ個々の処理について理解できていないため、パターンから外れたことで混乱しているように見受けられます。 ヒントとして、 最新の情報を取得するボタンを簡単に実装するならこんな感じのものになると思います。 <%= button_to '最新の情報を取得', fetch_items_feeds_path %> FeedsController < ApplicationController
# (省略)
# 最新の情報を取得ボタンを押して呼ばれるアクション
def fetch_items
# feed#url のRSSから記事情報を登録して Item (Feed#items) に追加する処理を書く。
# データを取ってくるところまでは Feed.channels でできているので、取ってきたデータをDBに入れることを考えればよい
redirect_to feeds_url, notice: '最新の情報を取得しました。'
end
end |
@itkrt2y 「一事実一箇所」という考え方を念頭に置いて自分の設計を見ると、無駄だらけの設計でした、、、:dizzy_face: |
@takeyuweb ヒント、ありがとうございます:bow: |
まず、テーブルの設計を修正していきます。(テーブル名の変更も含め feed → channel) |
一旦テーブル修正などを行い、pushしました。テスト(CircleCI)のエラーは調査中です。(外部キー関連のようで、、、)test_helper.rbの fixtures :all をコメントアウトすると通りますが、果たしてそれが良いことなのかも調査中です。。。:cold_sweat: |
@@ -0,0 +1,6 @@ | |||
class RenameFeedsToChannels < ActiveRecord::Migration[5.2] | |||
def change | |||
rename_table :channels, :items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
そもそも create_items
でmigrationを作れば、このrenameも不要
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
既存のchannelsテーブルの内容を修正して、新しくitemsテーブルを作り、機を見て(チーム開発なら)feedsテーブルを削除という順序だったということでしょうか?:sweat_drops:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
いや、以下の流れですね。削除いらない
- feeds => channelsにrename
- channelsテーブルにカラム追加
- itemsテーブル作成
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feedsテーブル, channelsテーブル(迷走して作っていたテーブル)の二つが存在していた状況だったので、feedsテーブルをrenameすると同じ名前のものが2つになるため、作れないのかと思ってしまっていました:sweat_drops:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
迷走中に作ったテーブルはdb:rollback
で消せばええんやで
まだマージされていなくて @shiraryu さんのところにしかないのでできることですが
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
そういうことだったんですね:sweat_drops:
マージされているかいないかの部分をもっと意識して作業します。
title, descriptionカラム追加の際に、新しくマイグレーションファイルを作って行わなかったことも含め、気をつけます:bow:
修正(整えて)いきます。
app/models/channel.rb
Outdated
@@ -1,3 +1,10 @@ | |||
class Channel < ApplicationRecord | |||
belongs_to :feed | |||
has_many :items, dependent: :destroy | |||
accepts_nested_attributes_for :items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これ、使ってないのでは
|
||
private | ||
def channel_params | ||
params.require(:channel).permit(:url, :title, :description, items_attributes: [:title, :link, :pubdate]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:url
以外について、なぜ追加で指定したのか、理由を説明できますか?
データ構造の修正まで、ということなら今はいいか
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
まあ、聞くに越したことはないかと。次のタスクにも影響しますし
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ふむ、では改めて
上で出た accepts_nested_attributes_for :items
も関連しますが、なぜこれらを追加したか理由を説明できますか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
いえ、説明できません:sweat_drops:
これらは「最新の情報を取得する」ボタンを作成するということを試みていた際に迷走した結果でして、、、:cold_sweat:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほど、ではいったん消して、先にヒントとして僕が出したものを実装していくうえで必要になったらその時に書いてください。
今はまだ難しいかもしれませんが、プログラミングにおいて『理由を説明できないコード』は基本的にあってはならないと思っていてください。
コードを読む際はコードの理由を考えるので、こういった理由を説明できないコードがあると理解が難しくなります。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ってコメントしましたが、この箇所については単に削除漏れと理解したので、消せばOKで、日頃そういうことを意識してコードを書くよう心がけてください、というお話です。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
承知しました!ただ、「ドキッ」としました。
「説明できないや」と。。。
つまり、「最新の情報を取得する」ボタンを作成する試みをしていた際に「当てずっぽう」なことをしていたということが問題ですね。『理由を説明できないコード』はあってはならないということを肝に命じておきます。ありがとうございます。
@@ -2,6 +2,9 @@ class CreateFeeds < ActiveRecord::Migration[5.2] | |||
def change | |||
create_table :feeds do |t| | |||
t.string :url, null: false, default: '', comment: 'RSSフィードのURL' | |||
t.string :title | |||
t.string :description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
一人プロジェクトなのでわかってやってるなら別にいい話になりますが、migrationって一度流れたものは繰り返し流れないので、仮に複数人でこの開発を行っていた場合、このmigrationが実行済みの人はこの変更を反映できなくなります。
新規にmigrationファイルを作成するのが正しい手順になります。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
開発云々よりも、本番にあげたのをmigration resetしなければならないのが最も問題か。データを全部消して再実行するしかない。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
承知しました!
今は消してもいいやつです。追々学んでいただければ |
データ構造はこれでOK |
@itkrt2y 「fixtures :all」「データ構造」の件、ありがとうございます! |
fixturesの件、ざっくり説明すると以下のようなお話
今はテスト書いてないですし、書くとしてもfixtures使うか微妙なので消しといてOK。真面目に対処するならば、fixtures定義をちゃんとやれば通るようになります。 |
@itkrt2y fixturesの説明、わかりやすくありがとうございます:bow: |
test/test_helper.rb
Outdated
@@ -4,7 +4,6 @@ | |||
|
|||
class ActiveSupport::TestCase | |||
# Setup all fixtures in test/fixtures/*.yml for all tests in alphabetical order. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このコメントは fixtures :all
の説明なので、これも消すべき
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
承知しました!
render 'new' | ||
end | ||
channel_id = @channel.id | ||
Channel.items_save(channel_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
少し理解しづらいかもしれませんが、クラスメソッドとインスタンスメソッドについて調べてみてください。
ruby インスタンスメソッド クラスメソッド
などとググれば参考になるものも見つかると思います。(この仕事、ググり力もかなり重要、鍛えましょう
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この行でもう一つ。
ここだと @channel.save
が失敗した、つまり、channelが存在しない場合においてもchannelのitemを保存しようとしてしまいます
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@takeyuweb 承知しました!調べていきます!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itkrt2y 本当ですね:sweat_drops: 修正します!
まだ、クラスメソッドとインスタンスメソッドを理解しづらい状況です。 |
app/models/channel.rb
Outdated
def self.items_save(channel_id) | ||
channel = find(channel_id) | ||
def items_save(channel_id) | ||
channel = Channel.find(channel_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
おっ、そうですね、これはChannel
クラスインスタンスのitems
を操作するので、Channel
クラスのインスタンスメソッドですね。
インスタンスメソッドを使うことで、特定の Channel
クラスインスタンスに対して、「items更新しろ」と指示できるわけですね。
一方でクラスメソッドは Channel
クラスには関係あるけど、個々のインスタンスのデータは使わないメソッドを定義したいときに使います。ちょっと今ちょうどいい例を思いつきませんが、どんな・誰のデータを扱うか、で使い分ける感じです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ただこれ、惜しいです。
ヒントは items_save
メソッド中における self
はなんでしょうね…というのは、ちょっとまわりくどいかな。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@takeyuweb すいません。コメントに気づかず、pushしてしまいました:sweat_drops:
再度、コメント内容を考えてみます!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shiraryu はい、そういうことです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@takeyuweb ご確認ありがとうございます!
まだモヤっとしているので、整理していきます:bow:
end | ||
|
||
def create | ||
@channel = Channel.new(channel_params) | ||
if @channel.save | ||
@channel.items_save(@channel.id) | ||
Channel.new.items_save(@channel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これじゃインスタンスメソッドにした意味ない
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
そこじゃないですっておもったけど惜しいところもあるか
end | ||
|
||
def create | ||
@channel = Channel.new(channel_params) | ||
if @channel.save | ||
Channel.new.items_save(@channel) | ||
@channel.items_save(@channel.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これはちょっと近づいた
もう一息
@@ -2,28 +2,27 @@ | |||
# https://news.yahoo.co.jp/pickup/rss.xml | |||
# https://rss-weather.yahoo.co.jp/rss/days/7320.xml | |||
class ChannelsController < ApplicationController | |||
before_action :set_channel,only:[:new, :fetch_items] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetch_items
で @channel = Channel.new
じゃだめだから、間違ってるし、
そもそもこれによって何も楽になってないから、わざわざ before_action
する必要も特にない
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
再度、調べ直します!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
調べるだけじゃ意味なくて、考えて理解する。
調べてそれっぽいのをコピペしたりするのは、あくまでも理解するまでの過程。
コピペするにしてもなぜ必要なのか?考えて説明できるようにしないとだめです。
コピペプログラマというのも世の中には存在するらしいですが。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
すいません、言葉が悪かったです。現時点でコピペは行っていません。
単純に理解が足りていない状況です。
「レシーバを操作する」って概念が全くわかっていない印象 |
クラスとインスタンスの違いも全然わかってないので、インスタンスメソッドでクラスメソッドな処理を書いている |
|
@itkrt2y はい、はっきりと「なるほど」というように違いを認識できずにいます。しかもそれほど意味不明なことをしてる状況でしたか、、、自身で言うのも何ですが、現時点で深刻だと感じるほど理解しにくいのは事実です。 |
クラスとインスタンスの違い、基本的な事柄すぎて逆に説明しづらいのですが、RailsのModelに限った話で例えると以下のような感じで、RailsのModelではクラスはテーブルを表し、インスタンスは1レコードを表している。 Channel.create(title: "Channel Title") # createはクラスメソッド。channelsテーブルにレコードを作る
channel = Channel.find(1) # findはクラスメソッド。channelsテーブルからレコードを取得する
channel.title = "Updated Title" # title= はインスタンスメソッド。channelインスタンスのtitileに値を代入する
channel.save # saveはインスタンスメソッド。channelインスタンスの現在の状態をDBに保存する Modelじゃなくても基本的にはこういう概念であり、「Dogクラス作って個々の犬を表すインスタンスを作る」みたいなことをする。 |
@itkrt2y ありがとうございます!「レシーバを操作する」ということと共に、再度落ち着いて考えます:bow: |
app/models/channel.rb
Outdated
|
||
items = channel.items | ||
items = self.items |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
変数に入れる必要ないと思います
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
承知しました!
app/models/channel.rb
Outdated
def items_save | ||
source_channel = RSS::Parser.parse(self.url).channel | ||
self.assign_attributes(title: source_channel.title, description: source_channel.description) | ||
self.save if self.changed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.
書いてるの、間違ってるというわけでもないのですが、省略できるものは書かないのが一般的で、これら self
は全部省略可能です。
self
が必要になるのは多分変数代入くらい
title = 'foo' # これはChannel#titleへの代入よりも変数定義が優先される
self.title = 'bar' # selfをつければChannel#itemへの代入になる
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
「省略できるものは基本書かない」承知しました!
変数定義と変数代入の違いも押さえておきます!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝ のコメント # selfをつければChannel#titleへの代入になる
ですね。
もうちょっと厳密にいえば、この場合は代入ではなくて Channel#title=
メソッドの呼び出し、でしょうか。
省略できるので不要ですが self.
を書いてるの、レシーバを意識できているという点では良い兆候です。
self.assign_attributes
は、 self.
つまり自分自身を .assign_attributes
メソッドで操作するよ、ということですね。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
クラスとインスタンスの違いはもとより、レシーバを意識することやインスタンスメソッド内のselfがオブジェクトとなっているということなど、わかっていないことが多かったと感じています:bow: selfが有るか無いかで大きく意味合いが変わる点、気をつけます:sweat_drops:
app/models/channel.rb
Outdated
has_many :items, dependent: :destroy | ||
validates :url, format: /\A#{URI::regexp(%w(http https))}\z/ | ||
|
||
def items_save |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
items_save
って名前だと"Items(S) save(V)"で次に目的語が期待されるのであまり正しくないですね。
update_items
くらいでいいかな
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
命名のことを意識もしていませんでした、、、
気をつけていきます:bow:
refs #4
[WIP]の使い方が間違っていたらすいません。アドバイスを頂いたにもかかわらずなかなか機能を追加することできないので、テーブル追加&アソシエーションの段階で一旦pushしました。
(板倉さんアドバイスを更に細分化しなければ僕は頭の整理できないのではと思いつつ)
(追加内容) 『最新の情報を取得』ボタン → 情報取得 → DBに保存 → /feeds で保存した情報表示
title/description/linkをDBに保存できるよう、テーブル(Model)を作成
RSSフィードを取ってきて、1で作ったDBに保存するactionを作成(『最新の情報を取得』ボタン)
(この際、複数回actionを実行しても同じ記事が複数登録されないように注意)
記事一覧をDBに保存しているものを表示するのに変更
RSSフィード登録時に、DBに最新記事を登録する処理を追加
定期的にRSSフィードを更新する処理を追加(cron。HerokuだとSchedulerというaddonを使うことになる)